Skip to content

fix(actions): run TransferLogs on UpdateLog{Rows:[], NoMore:true}#37631

Merged
silverwind merged 8 commits into
go-gitea:mainfrom
silverwind:fix-dbfs-data-leak
May 13, 2026
Merged

fix(actions): run TransferLogs on UpdateLog{Rows:[], NoMore:true}#37631
silverwind merged 8 commits into
go-gitea:mainfrom
silverwind:fix-dbfs-data-leak

Conversation

@silverwind
Copy link
Copy Markdown
Member

@silverwind silverwind commented May 10, 2026 •
edited
Loading

UpdateLog short-circuits on len(Rows)==0 before honoring NoMore, so a final empty UpdateLog{NoMore:true} never runs TransferLogs. The task's dbfs_data rows are then never moved to log storage and never deleted.

The bug has been latent since the original Actions implementation, act_runner versions after runner#819 trip it deterministically.

Fix: let NoMore=true with no new rows fall through to TransferLogs. Bail when the runner has outrun the server (Index > ack) even with NoMore, since archiving a log with a gap is worse than retrying. Always call WriteLogs so offset==0 bootstraps an empty DBFS file in the no-output case (otherwise TransferLogs would fail at dbfs.Open).

Fixes: #37623
Ref: runner#952
Ref: runner#950

This PR was written with the help of Claude Opus 4.7

@silverwind @claude
fix(actions): run TransferLogs on UpdateLog{Rows:[], NoMore:true}
b0e5102 
UpdateLog short-circuits on len(Rows)==0 before honoring NoMore, so a
final empty UpdateLog{NoMore:true} request from a runner that has
already streamed all its rows never runs TransferLogs. As a result,
dbfs_data rows for that task are never moved to the log filesystem and
never deleted. The latent bug has existed since the original Actions
implementation in 4011821 (2023-01-31); act_runner versions after
0.3.1 trip it deterministically because they flush eagerly on the
job-result event, leaving the buffer empty at Close() time.

Extract the decision logic into planLogUpdate() and let NoMore=true
requests with no new rows fall through to the TransferLogs branch. Bail
when the runner has outrun the server (Index > ack) even with NoMore,
since archiving a log with a gap is worse than asking the runner to
retry. Add a unit test for the helper.

Refs gitea/runner#950.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 10, 2026
@silverwind silverwind mentioned this pull request May 10, 2026
Closed
@silverwind silverwind requested a review from Copilot May 10, 2026 10:50
Copilot AI reviewed May 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a long-standing Actions runner log finalization edge case where a final UpdateLog{Rows: [], NoMore: true} request could be short-circuited, preventing TransferLogs() from running and leaving dbfs_data rows orphaned (unbounded DB growth).

Changes:

  • Extracted the log-update decision logic into planLogUpdate() to correctly handle empty finalization requests and “runner ahead of server” scenarios.
  • Updated UpdateLog to write only the unacked rows and to allow NoMore=true with no new rows to still finalize/transfer logs.
  • Added unit tests covering planLogUpdate() behavior, including the regression scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
routers/api/actions/runner/runner.go Refactors UpdateLog control flow via planLogUpdate() and ensures NoMore requests can trigger finalization even when Rows is empty.
routers/api/actions/runner/runner_test.go Adds unit tests for planLogUpdate() including regression coverage for empty finalization behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread routers/api/actions/runner/runner.go Outdated
silverwind and others added 3 commits May 10, 2026 12:58
@silverwind @claude
fix: bootstrap empty DBFS file when finalizing a logless task, fmt im…
d2a45c8 
…ports

If a runner sends a single UpdateLog{Rows:[], NoMore:true} for a task
that never wrote any logs, the previous fix would skip WriteLogs entirely
and TransferLogs would fail at dbfs.Open with os.ErrNotExist. Drop the
len(newRows) > 0 guard around WriteLogs: with offset==0 and empty rows,
WriteLogs creates an empty DBFS file via O_CREATE; with a non-zero
offset and empty rows it's a cheap no-op. TransferLogs then has a file
to read in the no-output case.

Also drop a blank line in runner_test.go imports to satisfy fmt-check.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind @claude
simplify: inline log update plan, drop helper and unit test
d257657 
The planLogUpdate helper was extracted only to enable a unit test for
the decision logic. Inlining it keeps the fix to the smallest possible
change against the original UpdateLog body. The rest of the package has
no unit tests for any handler, so dropping the test is consistent with
existing conventions; the integration tests still cover the happy path.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind @claude
restore minimal unit test for log update bail logic
e642b8a 
Reintroduce a tiny planLogUpdate helper (returns trimmed rows + bail
bool, no separate finalize) and a 4-case test covering: the regression
fix, the preserved !NoMore bail, the trim happy path, and the
runner-ahead safety bail.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
bircni
bircni reviewed May 10, 2026
View reviewed changes
Comment thread routers/api/actions/runner/runner.go Outdated
bircni
bircni reviewed May 10, 2026
View reviewed changes
Comment thread routers/api/actions/runner/runner_test.go Outdated
silverwind and others added 2 commits May 10, 2026 22:48
@silverwind @claude
test(actions): replace planLogUpdate unit test with integration test
c04f179 
Inlines the planLogUpdate helper back into UpdateLog and drops the
unit test. Adds an integration test that triggers a workflow, fetches
the task, sends UpdateLog{Rows:[], NoMore:true} directly via the runner
client, and asserts the three regression signals: task.LogInStorage
flips, archive is in storage, dbfs_data row is removed. This exercises
the actual gitea/runner#950 reproducer path rather than just the bail
decision.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind @claude
Simplify finalize-without-rows test
72e2d7d 
- use actions_module.DBFSPrefix instead of literal "actions_log/"
- align local names with codebase convention (wfTreePath, wfFileContent)
- dedupe the workflow path string via fmt.Sprintf
- drop the trailing UpdateTask{SUCCESS} call: TransferLogs and the dbfs remove() already ran synchronously inside UpdateLog, so the regression signals are observable without finalizing the task state
- require.True on LogInStorage: the next two assertions are noise if it's false

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind silverwind requested a review from Copilot May 10, 2026 21:08
Copilot AI reviewed May 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@lunny lunny added the type/bug label May 11, 2026
@bircni
Copy link
Copy Markdown
Member

bircni commented May 11, 2026

@silverwind you wanna backport this?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 11, 2026
@silverwind
Copy link
Copy Markdown
Member Author

silverwind commented May 11, 2026 •
edited
Loading

Could backport but not strictly needed because the runner fix is merged and serves as a acceptable workaround. If we backport, we could remove the runner workaround with 1.27, so imho still valuable to not have that workaround laying around for 3+ months.

@silverwind silverwind added the backport/v1.26 This PR should be backported to Gitea 1.26 label May 11, 2026
chhe pushed a commit to chhe/act_runner that referenced this pull request May 11, 2026
@silverwind
fix: ensure dbfs_data is cleaned up after task completion (#952)
295eecb 
Fixes #950.

After #819, the daemon flushes logs eagerly on the job-result entry (via the `stateNotify` path), so `Close()` typically runs `ReportLog(true)` with an empty buffer. Gitea's `UpdateLog` handler short-circuits on `len(Rows)==0` before honoring `NoMore`, so the final request never runs `TransferLogs` and `dbfs_data` rows leak. The server-side short-circuit is latent since the original Actions implementation in 2023; #819 made it deterministically reachable.

Workaround: inject a sentinel row in `Close()` after the daemon has exited so the final `UpdateLog` always carries at least one row. Done after the daemon waits so the sentinel can't be flushed before `ReportLog(true)` reads it.

go-gitea/gitea#37631 drops the empty-rows short-circuit when `NoMore=true`; that would work with or without this PR.

Reviewed-on: https://gitea.com/gitea/runner/pulls/952
Reviewed-by: Nicolas <bircni@icloud.com>
Reviewed-by: Zettat123 <39446+zettat123@noreply.gitea.com>
Co-authored-by: silverwind <me@silverwind.io>
Co-committed-by: silverwind <me@silverwind.io>
@bircni bircni mentioned this pull request May 11, 2026
Closed
25 tasks
@bircni bircni requested review from lunny May 11, 2026 17:43
@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 13, 2026
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label May 13, 2026
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 13, 2026
@silverwind silverwind enabled auto-merge (squash) May 13, 2026 04:52
@silverwind silverwind merged commit 79f7062 into go-gitea:main May 13, 2026
21 checks passed
@silverwind silverwind deleted the fix-dbfs-data-leak branch May 13, 2026 05:18
@GiteaBot GiteaBot added this to the 1.27.0 milestone May 13, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 13, 2026
@GiteaBot GiteaBot mentioned this pull request May 13, 2026
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label May 13, 2026
silverwind added a commit that referenced this pull request May 13, 2026
@GiteaBot @silverwind @claude
fix(actions): run TransferLogs on UpdateLog{Rows:[], NoMore:true} (…
28729ef 
…#37631) (#37687)

Backport #37631 by @silverwind

`UpdateLog` short-circuits on `len(Rows)==0` before honoring `NoMore`,
so a final empty `UpdateLog{NoMore:true}` never runs `TransferLogs`. The
task's `dbfs_data` rows are then never moved to log storage and never
deleted.

The bug has been latent since the original Actions implementation,
`act_runner` versions after
[runner#819](https://gitea.com/gitea/runner/pulls/819) trip it
deterministically.

Fix: let `NoMore=true` with no new rows fall through to `TransferLogs`.
Bail when the runner has outrun the server (`Index > ack`) even with
`NoMore`, since archiving a log with a gap is worse than retrying.
Always call `WriteLogs` so `offset==0` bootstraps an empty DBFS file in
the no-output case (otherwise `TransferLogs` would fail at `dbfs.Open`).

Fixes: #37623
Ref: [runner#952](https://gitea.com/gitea/runner/pulls/952)
Ref: [runner#950](https://gitea.com/gitea/runner/pulls/950)

---
This PR was written with the help of Claude Opus 4.7

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com>
silverwind added a commit to silverwind/gitea that referenced this pull request May 13, 2026
@silverwind
Merge remote-tracking branch 'origin/main' into lint-shell-merge
a37ba13 
* origin/main:
  chore: clean up "contrib" dir (go-gitea#37690)
  feat(api): add sort and order query parameters to job list endpoints (go-gitea#37672)
  fix: Sort action run jobs by JobID and Name with matrix examples (go-gitea#37046)
  fix: catch and fix more lint problems (go-gitea#37674)
  docs(agents): update AGENTS.md (go-gitea#37684)
  fix(actions): run `TransferLogs` on `UpdateLog{Rows:[], NoMore:true}` (go-gitea#37631)

# Conflicts:
#	build/test-env-check.sh
#	contrib/update_dependencies.sh
silverwind added a commit to bircni/gitea that referenced this pull request May 14, 2026
@silverwind
Merge remote-tracking branch 'origin/main' into feature/display-co-au…
44ae7cd 
…thor

* origin/main:
  [skip ci] Updated translations via Crowdin
  fix: snap build (main branch) (go-gitea#37685)
  chore: introduce HTMLBuilder (go-gitea#37688)
  chore: clean up "contrib" dir (go-gitea#37690)
  feat(api): add sort and order query parameters to job list endpoints (go-gitea#37672)
  fix: Sort action run jobs by JobID and Name with matrix examples (go-gitea#37046)
  fix: catch and fix more lint problems (go-gitea#37674)
  docs(agents): update AGENTS.md (go-gitea#37684)
  fix(actions): run `TransferLogs` on `UpdateLog{Rows:[], NoMore:true}` (go-gitea#37631)
eleboucher pushed a commit to eleboucher/apoci that referenced this pull request May 20, 2026
@eleboucher
fix(deps): update module code.gitea.io/gitea (v1.26.1 → v1.26.2) (#47)
ce5a949 
This PR contains the following updates:

| Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [code.gitea.io/gitea](https://github.com/go-gitea/gitea) | `v1.26.1` → `v1.26.2` | ![age](https://developer.mend.io/api/mc/badges/age/go/code.gitea.io%2fgitea/v1.26.2?slim=true) | ![confidence](https://developer.mend.io/api/mc/badges/confidence/go/code.gitea.io%2fgitea/v1.26.1/v1.26.2?slim=true) |

---

### Release Notes

<details>
<summary>go-gitea/gitea (code.gitea.io/gitea)</summary>

### [`v1.26.2`](https://github.com/go-gitea/gitea/releases/tag/v1.26.2)

[Compare Source](go-gitea/gitea@v1.26.1...v1.26.2)

- SECURITY
  - fix(permissions): Fix reading permission ([#&#8203;37769](go-gitea/gitea#37769))
  - fix(actions): make artifact signature payloads unambiguous ([#&#8203;37707](go-gitea/gitea#37707))
  - fix: Unify public-only token filtering in API queries and repo access checks ([#&#8203;37118](go-gitea/gitea#37118))
  - fix: Add missed token scope checking ([#&#8203;37735](go-gitea/gitea#37735))
  - fix(oauth): bind token exchanges to the original client request ([#&#8203;37704](go-gitea/gitea#37704))
  - fix(oauth): strengthen PKCE validation and refresh token replay protection ([#&#8203;37706](go-gitea/gitea#37706))
  - fix(web): enforce token scopes on raw, media, and attachment downloads ([#&#8203;37698](go-gitea/gitea#37698))
  - fix(security): enforce wiki git writes and LFS token access at request time ([#&#8203;37695](go-gitea/gitea#37695))
  - feat(api): encrypt AWS creds ([#&#8203;37679](go-gitea/gitea#37679))
  - fix(deps): update dependency mermaid to v11.15.0 \[security], add e2e test
  - fix(packages): Add label for private and internal package and fix composor package source permission check ([#&#8203;37610](go-gitea/gitea#37610))
  - fix(git): Fix smart http request scope bug ([#&#8203;37583](go-gitea/gitea#37583))
  - Fix basic auth bug ([#&#8203;37503](go-gitea/gitea#37503))
  - Fix allow maintainer edit permission check ([#&#8203;37479](go-gitea/gitea#37479)) ([#&#8203;37484](go-gitea/gitea#37484))
  - Fix URL sanitization to handle schemeless credentials ([#&#8203;37440](go-gitea/gitea#37440)) ([#&#8203;37471](go-gitea/gitea#37471))
  - Fix attachment Content-Security-Policy ([#&#8203;37455](go-gitea/gitea#37455)) ([#&#8203;37464](go-gitea/gitea#37464))
  - chore(deps): bump go-git/go-git/v5 to 5.19.0 ([#&#8203;37608](go-gitea/gitea#37608))

- BUGFIXES
  - fix(pull): handle empty pull request files view to allow reviews ([#&#8203;37783](go-gitea/gitea#37783))
  - fix(markup): make RenderString never fail ([#&#8203;37779](go-gitea/gitea#37779))
  - fix: add natural sort to sortTreeViewNodes ([#&#8203;37772](go-gitea/gitea#37772))
  - fix: package creation unique conflict ([#&#8203;37774](go-gitea/gitea#37774))
  - fix!: add DEFAULT\_TITLE\_SOURCE setting for pull request title default behavior ([#&#8203;37465](go-gitea/gitea#37465))
  - fix: Allow direct commits for unprotected files with push restrictions ([#&#8203;37657](go-gitea/gitea#37657))
  - fix(actions): wrong assumption that run id always >= job id ([#&#8203;37737](go-gitea/gitea#37737))
  - fix(auth): set User-Agent on avatar fetch and sync avatar on link-account register ([#&#8203;37564](go-gitea/gitea#37564)) ([#&#8203;37588](go-gitea/gitea#37588))
  - fix(actions): deadlock between PrepareRunAndInsert and UpdateTaskByState ([#&#8203;37692](go-gitea/gitea#37692))
  - fix(repo): /generate must sync the branch table for the new repo ([#&#8203;37693](go-gitea/gitea#37693))
  - build: Fix snap build (1.26)
  - fix(actions): run TransferLogs on UpdateLog{Rows:\[], NoMore:true} ([#&#8203;37631](go-gitea/gitea#37631))
  - fix show correct mergebase
  - fix: make clone URL respect public URL detection setting ([#&#8203;37615](go-gitea/gitea#37615))
  - fix: "run as root" check ([#&#8203;37622](go-gitea/gitea#37622))
  - chore(deps): update dependency go to v1.26.3 ([#&#8203;37601](go-gitea/gitea#37601))
  - Compare dropdown fails when selecting branch with no common merge-base ([#&#8203;37470](go-gitea/gitea#37470))
  - fix: treat email addresses case-insensitively ([#&#8203;37600](go-gitea/gitea#37600))
  - fix(actions): fix blank lines after ::endgroup:: ([#&#8203;37597](go-gitea/gitea#37597))
  - fix(actions): report individual step status in workflow job API response ([#&#8203;37592](go-gitea/gitea#37592))
  - fix: Invalid UTF-8 commit messages in JSON API responses ([#&#8203;37542](go-gitea/gitea#37542))
  - fix: use consistent GetUser family functions ([#&#8203;37553](go-gitea/gitea#37553))
  - fix(api): return 409 message instead of empty JSON for wrong commit id ([#&#8203;37572](go-gitea/gitea#37572))
  - fix(actions): prevent panic when workflow contains null jobs ([#&#8203;37570](go-gitea/gitea#37570))
  - Make ServeSetHeaders default to download attachment if filename exists ([#&#8203;37552](go-gitea/gitea#37552)) ([#&#8203;37555](go-gitea/gitea#37555))
  - Fix(actions): validate workflow param to prevent 500 error ([#&#8203;37546](go-gitea/gitea#37546)) ([#&#8203;37554](go-gitea/gitea#37554))
  - Don't unblock run-level-concurrency-blocked runs in the resolver ([#&#8203;37461](go-gitea/gitea#37461)) ([#&#8203;37538](go-gitea/gitea#37538))
  - Fix(packages): use file names for generic web downloads ([#&#8203;37514](go-gitea/gitea#37514)) ([#&#8203;37520](go-gitea/gitea#37520))
  - Fix merge autodetect can't close other PRs but only the last one when multiple PRs are pushed at once ([#&#8203;37512](go-gitea/gitea#37512)) ([#&#8203;37516](go-gitea/gitea#37516))
  - Fix update branch protection order ([#&#8203;37508](go-gitea/gitea#37508)) ([#&#8203;37513](go-gitea/gitea#37513))
  - Fix mCaptcha broken after Vite migration ([#&#8203;37492](go-gitea/gitea#37492)) ([#&#8203;37509](go-gitea/gitea#37509))
  - Fix review submission from single-commit PR view ([#&#8203;37475](go-gitea/gitea#37475)) ([#&#8203;37485](go-gitea/gitea#37485))
  - Fix scheduled action panic with null event payload ([#&#8203;37459](go-gitea/gitea#37459)) ([#&#8203;37466](go-gitea/gitea#37466))
  - Make GetPossibleUserByID can handle deleted user ([#&#8203;37430](go-gitea/gitea#37430)) ([#&#8203;37431](go-gitea/gitea#37431))
  - Remove excessive quote from terraform instructions ([#&#8203;37424](go-gitea/gitea#37424)) ([#&#8203;37426](go-gitea/gitea#37426))
  - Fix color regressions, add `priority` color ([#&#8203;37417](go-gitea/gitea#37417)) ([#&#8203;37421](go-gitea/gitea#37421))

- MISC
  - Add CurrentURL template variable back ([#&#8203;37444](go-gitea/gitea#37444)) ([#&#8203;37449](go-gitea/gitea#37449))

Instances on **[Gitea Cloud](https://cloud.gitea.com)** will be automatically upgraded to this version during the specified maintenance window.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJ0eXBlL3BhdGNoIl19-->

Reviewed-on: https://git.erwanleboucher.dev/eleboucher/apoci/pulls/47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Zettat123 Zettat123 Zettat123 approved these changes

@bircni bircni bircni approved these changes

@lunny lunny Awaiting requested review from lunny

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

The dbfs_data table is not being properly cleaned up (not written back to the regular file system)

6 participants

@silverwind @bircni @Zettat123 @lunny @GiteaBot